Asciidoctor: Add support for inline callouts #566

nik9000 · 2019-01-30T21:34:58Z

Adds support for inline callout in a fairly hacky way. These look like:

----
POST <1> /_search/scroll <2>
----
<1> words
<2> other words

They are supported by asciidoc and the elasticsearch reference uses a
few of them and they look great. Asciidoctor doesn't support them and
doesn't have a nice extension point to add them so we hack asciidoctor
fairly shamelessly to get them in.

This cuts the number of failures when building the Elasticsearch
reference down from "then entire screen" to 3.

Adds support for inline callout in a fairly hacky way. These look like: ``` ---- POST <1> /_search/scroll <2> ---- <1> words <2> other words ``` They are supported by asciidoc and the elasticsearch reference uses a few of them and they look *great*. Asciidoctor doesn't support them and doesn't have a nice extension point to add them so we hack asciidoctor fairly shamelessly to get them in. This cuts the number of failures when building the Elasticsearch reference down from "then entire screen" to 3.

ddillinger

Left some questions here and there, overall I feel probably less offended than the comments seem like they expect. It would be ideal if upstream supported the feature instead though.

ddillinger · 2019-01-30T22:26:14Z

resources/asciidoctor/spec/inline_callout_spec.rb

+RSpec.describe InlineCallout do
+  it "enables support for inline callouts if requested" do
+    attributes = {
+      'inline-callouts' => '',


What meaning does '' have as a value? In many languages that value is false-y.

In asciidoctor flags attributes are usually controlled by being defined at all. It is weird, but it feels more like the way the rest of asciidoctor works.

ddillinger · 2019-01-30T22:27:50Z

resources/asciidoctor/lib/inline_callout/extension.rb

+#   POST <1> /_search/scroll <2>
+#
+# NOTE: This isn't an asciidoctor extension so much as a hack. Just including
+# the file causes us to hack asciidoctor to enable the behavior. By default we


Can this be more descriptive than just saying "hack" twice?

ddillinger · 2019-01-30T22:29:58Z

resources/asciidoctor/lib/inline_callout/extension.rb

+  # to get this into Asciidoctor proper. These methods are basically the same
+  # as the methods in asciidoctor but with new regexes.
+  old_verbose = $VERBOSE
+  $VERBOSE = false


What is the actual scope of this change? If this extension is loaded, does the altering of logging only occur for inline callouts, or is loading this extension going to affect logging levels for the duration of the run? I'm afraid I know too little about ruby evaluation and loading rules to just know this.

It is global but I restore it after the fact. I think threads could be an issue here, but we're super single threaded at this point. I don't like it much though.

I thought for a while about asking if it could be guarded by a check for the inline-callouts argument, but then I think I have decided that basically we're always going to have that on, because we use them (or else this extension wouldn've have been needed to begin with) so it doesn't matter for us.

Which in turn means that we are always going to have log warnings suppressed, which is why I am so concerned about understanding the scope of how much time a complete run spends with the log levels tampered with vs. not.

Which brings me to ask whether patching instances is preferable, possibly in combination with moving the verbose tweak inside the overridden method, to ensure the scope is exactly what you're really after?

ddillinger · 2019-01-30T22:34:19Z

resources/asciidoctor/lib/inline_callout/extension.rb

+      autonum = 0
+      callout_rx = (document.attr? 'inline-callouts') ? InlineCalloutScanRx : CalloutScanRx
+      text.scan(callout_rx) {
+        # lead with assignments for Ruby 1.8.7 compat


I doubt 1.8.7 is going to be a concern when asciidoctor requires >= 2.3

And if the answer is that we use 1.8.7 somewhere then I'm going to go ahead now and ask what our plan is to stop that.

This comes from lifting stuff out of asciidoctor. I'll clean it up.

Honestly I like the line even without 1.8.7 because it is easier to read. I think, at least.

nik9000

I've pushed a couple of cleanups and answered your questions.

It does look like monkeypatching is "the way to do this" now.

nik9000 · 2019-01-31T19:34:47Z

resources/asciidoctor/lib/inline_callout/extension.rb

+      autonum = 0
+      callout_rx = (document.attr? 'inline-callouts') ? InlineCalloutScanRx : CalloutScanRx
+      text.scan(callout_rx) {
+        # lead with assignments for Ruby 1.8.7 compat


Honestly I like the line even without 1.8.7 because it is easier to read. I think, at least.

ddillinger

Since this extension is effectively addressing a difference of behavior between asciidoc and asciidoctor, let me raise this question:

What is the asciidoctor recommended way to achieve what the documentation is trying to do when using this feature?

Put another way, can we get around supporting this by not needing it anymore?

ddillinger · 2019-02-01T14:11:34Z

resources/asciidoctor/lib/inline_callout/extension.rb

+# NOTE: This isn't an asciidoctor extension so much as a hack. Just including
+# the file causes us to monkey patch its code into asciidoctor. By  default we
+# don't change asciidoctor, but if you set the `inline-callouts` attribute so
+# you need to *ask* for the change in behavior.


I know I am kind of reviewing with the -pedantic flag on here but... I still have some issues.

The new sentence no longer really makes sense.

By default we don't change asciidoctor, but if you set the inline-callouts attribute so you need to ask for the change in behavior.

If it helps read it out loud. It's not conveying a coherent meaning.

It kind of tells a lie.

By default we don't change asciidoctor

As also discussed below, logging behavior is being altered too, and that behavior change is not guarded by any check against (attr? 'inline-callouts') either, it always does it.

The logging behavior is only altered for the duration of the monkey patching which is right at startup. We restore the old verbose mode after completing the patch.

I'll see about rewriting the sentence.

ddillinger · 2019-02-01T14:18:02Z

resources/asciidoctor/lib/extensions.rb

@@ -6,6 +6,9 @@
 require_relative 'elastic_compat_preprocessor/extension'
 require_relative 'elastic_include_tagged/extension'

+# This extensions is special because it is evil - just loading it is enough
+require_relative 'inline_callout/extension'


Then is it possible to only even load it when inline-callouts is defined?

It'd be possible but strange because we load these before we have a document to check what is defined. We'd have to load late which is indeed strange.

ddillinger · 2019-02-01T14:20:57Z

resources/asciidoctor/lib/inline_callout/extension.rb

+      found = false
+      autonum = 0
+      callout_rx = (document.attr? 'inline-callouts') ? InlineCalloutScanRx : CalloutScanRx
+      text.scan(callout_rx) {


Is the body here duplicated from what you are patching over? In particular if inline-callouts is not specified, I mean. Point being, in the absence of inline-callouts can this delegate to the normal implementation? (I'm probably showing how much ruby I don't really do, here...)

I can delegate, yeah. I wasn't partially because I figured this might be what it'd look like to make the change in asciidoctor itself. But it looks like they won't want this change until at least 2.0, which won't look like this anyway. So I can just delegate.

ddillinger · 2019-02-01T14:23:07Z

resources/asciidoctor/lib/inline_callout/extension.rb

+      }
+    end
+
+    def callout_rx


I freely admit this is picky, but this is both a) defined after it is used, and b) defined after a previous block of code also defined a different variable with the exact same name. I know it made me have to squint at it twice as a result. Presuming the order doesn't matter, and I'd be horrified if it did, I think it would read more clearly if it were swapped in order with sub_callouts.

The change is actually to a different place. This one is in Substitutors module and the other is in the Parser class. I have them in this order because it is the order in which they are run so it made sense to me. Would it be less confusing if the method were a variable? I can do that, it just didn't look as nice to me.

ddillinger · 2019-02-01T14:26:34Z

resources/asciidoctor/lib/inline_callout/extension.rb

+          # use sub since it might be behind a line comment
+          $&.sub(RS, '')
+        else
+          Inline.new(self, :callout, $4 == '.' ? (autonum += 1).to_s : $4, :id => @document.callouts.read_next_id, :attributes => { 'guard' => $1 }).convert


This line is too long and too complicated, it has a ?: inside the argument list, it uses magic gsub numbers, it has a hash assignment inside of a hash assignment...

Formatting may be enough to address most of this, a variable or two might also be helpful.

I comes from upstream so I dind't touch it.

I wouldn't have written it this way though.

ddillinger · 2019-02-01T14:29:48Z

resources/asciidoctor/lib/inline_callout/extension.rb

+  # to get this into Asciidoctor proper. These methods are basically the same
+  # as the methods in asciidoctor but with new regexes.
+  old_verbose = $VERBOSE
+  $VERBOSE = false


I thought for a while about asking if it could be guarded by a check for the inline-callouts argument, but then I think I have decided that basically we're always going to have that on, because we use them (or else this extension wouldn've have been needed to begin with) so it doesn't matter for us.

Which in turn means that we are always going to have log warnings suppressed, which is why I am so concerned about understanding the scope of how much time a complete run spends with the log levels tampered with vs. not.

Which brings me to ask whether patching instances is preferable, possibly in combination with moving the verbose tweak inside the overridden method, to ensure the scope is exactly what you're really after?

lcawl · 2019-02-01T17:36:04Z

What is the asciidoctor recommended way to achieve what the documentation is trying to do when using this feature?
Put another way, can we get around supporting this by not needing it anymore?

Per https://asciidoctor.org/docs/user-manual/#callouts "The callout number (at the target) must be placed at the end of the line."

In my opinion, the value of inline callouts is not high. We could fix the source files to have both callouts at the end of the line as asciidoctor requires.

nik9000 · 2019-02-02T02:11:38Z

In my opinion, the value of inline callouts is not high. We could fix the source files to have both callouts at the end of the line as asciidoctor requires.

I think it works pretty well in places like sql's percentile rank docs. But for the most part I don't want to have to go and rewrite docs in a bunch of branches just to be compatible with Asciidoctor. Maybe it is fine in this case, but I've been trying to avoid it in general because I feel like it'd be very hard to enforce until we switch to asciidoctor which gets us better tools.

ddillinger · 2019-02-04T14:21:46Z

I feel like I kind of disagree though. Given that asciidoc is EOL and all users are directed to move to asciidoctor, excessive* backward compat work toward a dead toolchain will only result in not only never returning to fix old idioms but also continuing to author more documentation source which also relies on a dead toolchain. Weaning off of things asciidoctor does not (and will not, and looking at the issue in their repo, this feature seems to fall in that category at least for the plan-able future) do is the best path out of that.

The value in these extensions is in a low-disruption transition period, but preventing ever transitioning off of asciidoc-not-asciidoctor is adding a needless code support burden in perpetuity for an infinitely diminishing return.

* The word 'excessive' is important here, it is crucial that we keep revisiting our understanding of where the line lies between "necessary and useful" and "too much".

Edit: if the deeper concern is that it is hard to enforce, we already have test builds for asciidoctor runs, we actually do have the capability to detect that case; the open question is how we want to communicate that during the time we're in transition and using asciidoc for the primary artifacts still.

nik9000 requested a review from ddillinger January 30, 2019 21:35

nik9000 mentioned this pull request Jan 30, 2019

Asciidoctor support #505

Closed

ddillinger reviewed Jan 30, 2019

View reviewed changes

nik9000 added 4 commits January 31, 2019 14:30

Merge branch 'master' into asciidoctor_funny_callouts

6dae474

Explain a little better

5b062ab

Drop note about 1.8.7

408053b

Drop extra

611a6e1

nik9000 commented Jan 31, 2019

View reviewed changes

ddillinger reviewed Feb 1, 2019

View reviewed changes

nik9000 closed this Feb 4, 2019

nik9000 mentioned this pull request Feb 4, 2019

Docs: Drop inline callouts elastic/elasticsearch#38335

Closed

This was referenced Feb 26, 2019

[DOCS] Fixes inline callout in TLS tutorial elastic/stack-docs#230

Merged

[DOCS] Fix inline callout in Watcher documentation elastic/elasticsearch#39423

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Asciidoctor: Add support for inline callouts #566

Asciidoctor: Add support for inline callouts #566

nik9000 commented Jan 30, 2019

ddillinger left a comment

ddillinger Jan 30, 2019

nik9000 Jan 31, 2019

ddillinger Jan 30, 2019

nik9000 Jan 31, 2019

ddillinger Jan 30, 2019

nik9000 Jan 31, 2019

ddillinger Feb 1, 2019

ddillinger Jan 30, 2019

nik9000 Jan 31, 2019

nik9000 Jan 31, 2019

nik9000 left a comment

nik9000 Jan 31, 2019

ddillinger left a comment

ddillinger Feb 1, 2019

nik9000 Feb 2, 2019

ddillinger Feb 1, 2019

nik9000 Feb 2, 2019

ddillinger Feb 1, 2019

nik9000 Feb 2, 2019

ddillinger Feb 1, 2019

nik9000 Feb 2, 2019

ddillinger Feb 1, 2019

nik9000 Feb 2, 2019

nik9000 Feb 2, 2019

ddillinger Feb 1, 2019

lcawl commented Feb 1, 2019

nik9000 commented Feb 2, 2019 •

edited

Loading

ddillinger commented Feb 4, 2019 •

edited

Loading

Asciidoctor: Add support for inline callouts #566

Asciidoctor: Add support for inline callouts #566

Conversation

nik9000 commented Jan 30, 2019

ddillinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nik9000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddillinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcawl commented Feb 1, 2019

nik9000 commented Feb 2, 2019 • edited Loading

ddillinger commented Feb 4, 2019 • edited Loading

nik9000 commented Feb 2, 2019 •

edited

Loading

ddillinger commented Feb 4, 2019 •

edited

Loading